Skip to content

api: add handling for xds.type.v3.TypedStruct#18567

Merged
mattklein123 merged 10 commits intoenvoyproxy:mainfrom
markdroth:xds_proto_upgrade2
Oct 13, 2021
Merged

api: add handling for xds.type.v3.TypedStruct#18567
mattklein123 merged 10 commits intoenvoyproxy:mainfrom
markdroth:xds_proto_upgrade2

Conversation

@markdroth
Copy link
Copy Markdown
Contributor

@markdroth markdroth commented Oct 11, 2021

Commit Message: api: add handling for xds.type.v3.TypedStruct
Additional Description: Adds handling for the new xds.type.v3.TypedStruct, but retains handling for legacy type udpa.type.v1.TypedStruct.
Risk Level: Low
Testing: Updated unit tests
Docs Changes: Included in PR
Release Notes: N/A
Platform Specific Features: N/A

Another step toward cncf/xds#2.

Signed-off-by: Mark D. Roth <roth@google.com>
Signed-off-by: Mark D. Roth <roth@google.com>
@repokitteh-read-only
Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #18567 was opened by markdroth.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added api deps Approval required for changes to Envoy's external dependencies labels Oct 11, 2021
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #18567 was opened by markdroth.

see: more, trace.

Signed-off-by: Mark D. Roth <roth@google.com>
Signed-off-by: Mark D. Roth <roth@google.com>
Signed-off-by: Mark D. Roth <roth@google.com>
This reverts commit 3c1eec6.

Signed-off-by: Mark D. Roth <roth@google.com>
Signed-off-by: Mark D. Roth <roth@google.com>
@markdroth markdroth marked this pull request as ready for review October 12, 2021 21:53
@markdroth
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18567 (comment) was created by @markdroth.

see: more, trace.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks LGTM. Can you add some type of release note about this?

/wait

Signed-off-by: Mark D. Roth <roth@google.com>
@markdroth
Copy link
Copy Markdown
Contributor Author

Release note added. PTAL. Thanks!

* ext_authz: added :ref:`query_parameters_to_set <envoy_v3_api_field_service.auth.v3.OkHttpResponse.query_parameters_to_set>` and :ref:`query_parameters_to_remove <envoy_v3_api_field_service.auth.v3.OkHttpResponse.query_parameters_to_remove>` for adding and removing query string parameters when using a gRPC authorization server.
* http: added support for :ref:`retriable health check status codes <envoy_v3_api_field_config.core.v3.HealthCheck.HttpHealthCheck.retriable_statuses>`.
* thrift_proxy: add upstream response zone metrics in the form ``cluster.cluster_name.zone.local_zone.upstream_zone.thrift.upstream_resp_success``.
* api: added support for *xds.type.v3.TypedStruct* in addition to the now-deprecated *udpa.type.v1.TypedStruct* proto message.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not going to pass format, it needs to be alpha sorted, can you move it to the top? Also, while you are here is it possible to add more color to this? I'm not sure most users are going to understand what this means. Not sure if there is anything that can be linked to, etc.

/wait

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only docs that I know of on this are the comments in the proto file in the cncf/xds repo. I've added an HTTP link to there, since I don't think there's a direct way to use an RST ref in this case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and it looks like RST isn't happy with the external HTTP link. So I don't think there's any reasonable way to add a link here, but I did leave a short explanation of what the proto message is for.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly the syntax I was using, but it failed the presubmit (log):

/tmp/tmpwbaogaun/generated/rst/version_history/current.rst:36: WARNING: Unknown target name: "a wrapper proto used to encode typed json data in a *google.protobuf.any* field<https://github.com/cncf/xds/blob/cb28da3451f158a947dfc45090fe92b07b243bc1/xds/type/v3/typed_struct.proto>".

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK pretty sure this should work but nbd, thanks for trying.

Signed-off-by: Mark D. Roth <roth@google.com>
Signed-off-by: Mark D. Roth <roth@google.com>
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mattklein123 mattklein123 merged commit 8af9f31 into envoyproxy:main Oct 13, 2021
@markdroth markdroth deleted the xds_proto_upgrade2 branch October 13, 2021 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deps Approval required for changes to Envoy's external dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants